Support for phpdbg with fast code coverage #258

JanTvrdik · 2015-10-02T07:39:16Z

Most of critical phpdbg bugs are fixed since RC4, some bugs are however still present.

https://bugs.php.net/bug.php?id=70532
https://bugs.php.net/bug.php?id=70531
https://bugs.php.net/bug.php?id=70614

Mikulas · 2015-10-02T08:17:02Z

🏆

(also it partially solves #241)

Majkl578 · 2015-10-02T19:53:19Z

src/CodeCoverage/Collector.php

+	 */
+	private static function startXdebug()
+	{
+		if (!extension_loaded('xdebug')) {


This should (imho) also check PHP_SAPI !== 'phpdbg'?

Why? Phpdbg is pointless when you use xdebux strategy. Did you meant to use or or and?

Majkl578 · 2015-10-02T20:00:50Z

👍

Since phpdbg seems to be much faster than XDebug (and it is also a native part of PHP), how about making it a default interpreter for PHP 7.0+?

If anyone is testing this, be aware that before PHP 7.0.0RC5, there is a bug in phpdbg coverage report when return types are used: php/php-src@6c61286.

milo · 2015-10-03T20:17:09Z

👍

Some Tester's tests are failing for me, I thing we should wait for final 7.0.0. In meantime:

is the ZendPhpDbgInterpreter needed? The only difference is the -qrr arg
I'm thinking about PhpInterpreter::hasXdebug() renaming, it's needed for coverage only
is the Coverage::STRATEGY_AUTO needed? Seems better to me send the strategy explicitly somehow from runner

JanTvrdik · 2015-10-03T20:30:21Z

Some Tester's tests are failing for me

You need RC4 to be able to run phpdbg and RC5 to make all tests OK.

The only difference is the -qrr arg

No.

I'm thinking about PhpInterpreter::hasXdebug() renaming

Or removing entirely. It's currently not used anywhere.

Seems better to me send the strategy explicitly somehow from runner

Runner may use different interpreter than jobs.

It's annoying to generate coverage now, because you need to write two long arguments (coverage + coverage-src). Feel free to add coverarage-strategy but the parameter must be optional. The autodetection is simple a will work OK for 99.9 % of people.

JanTvrdik · 2015-10-03T20:33:13Z

Thinking about it – it will probably be even better to split ZendPhpInterpreter to ZendPhpCgiInterpreter and ZendPhpCliInterpreter.

milo · 2015-10-03T21:25:40Z

Some Tester's tests are failing for me

You need RC4 to be able to run phpdbg and RC5 to make all tests OK.

Oh, I cloned from GitHub, there is no RC5 yet.

The only difference is the -qrr arg

No.

Hmm, --version works for both.

Seems better to me send the strategy explicitly somehow from runner

Runner may use different interpreter than jobs.

Can be detected in an /info.php way.

Majkl578 · 2015-10-16T12:54:12Z

7.0.0 RC5 is out.

hrach · 2015-11-09T09:45:46Z

👍 Is there any drawback/issue that needs to be solved?

milo · 2015-11-09T09:47:39Z

Waiting for stable 7 at least.

JanTvrdik · 2015-11-09T09:48:43Z

If anybody is willing to help – download the PR and run tests against latest PHP 7, last time I checked this one test was failing. Unfortunately I did not have the time to determine whether it is bug in tester or PHP 7.

hrach · 2015-11-09T10:19:16Z

Waiting for stable 7 at least.

Well, I do not want to push on merging a feature with failing tests, however, I see no reason not to merge a feature for an unstable version of php into development branch. Nette is full of support for php7.

Majkl578 · 2015-11-09T11:16:29Z

however, I see no reason not to merge a feature for an unstable version of php into development branch

Definitely 👍, that argument makes no sense to me.

Btw. PHP 7 release is postponed at least until the end of november.

milo · 2015-11-09T12:10:18Z

RC5 was not failling, later is failing. I don't want to invest my time into bug hunting. It's not only about Tester code, but compiling next RC and setup environment... that's time consuming.

That's the reason why I'm waiting for stable release.

JanTvrdik · 2015-11-09T13:44:58Z

I don't want to invest my time into bug hunting.

Ehm… now you can fix phpdbg before 7.0.0 released.

why I'm waiting for stable release

It will not start working unless someone will make it work. Waiting is pointless. The situation will only get worse.

milo · 2015-11-09T13:55:46Z

@JanTvrdik Please, delete the vulgar words.

Anyway, if you promise me that you will care about phpdbg and you will solve reported bugs about it, I'll invest the time into it.

hrach · 2015-11-09T13:57:34Z

I didn't want to bring emotions here. Still, the "at least" seems to be restrictive and impropper. :) That's all.

Majkl578 · 2015-11-09T13:59:08Z

@milo: Although I understand your approach which it makes sense for a library, unfortunately it's not that logical for a tool that is used to run tests. Especially when talking about php-next, which is pretty critical now as the GA is around the corner.

And in meantime, PHPUnit has PHP 7 support (incl. phpdbg) for over a month now...

JanTvrdik · 2015-11-09T14:17:47Z

@milo I can't promise that all bugs will be resolved before 7.0.0. I'm saying that they might be fixed. After the release you would need to learn how to time travel.

I understand your approach which it makes sense for a library

I don't. If you want your library to support PHP 7 you should always start working on that before PHP 7 is released not after. This is especially important for libraries which use low-level or uncommon APIs (such as Tester or Tracy).

dg · 2015-11-09T16:54:14Z

src/CodeCoverage/Collector.php

@@ -13,6 +13,11 @@
 */
 class Collector
 {
+	const


What about two Collector classes?

There is some common code, so you would need three classes – one for each strategy and one base class. The logic for deciding which strategy to use would probably end-up extracted outside of those classes in \Tester\Environment::setupCoverage.

To summarize – it would require more code but should be possible.

Mikulas · 2015-12-06T16:35:05Z

Ok so the tests fail on 7.0.0 as well.

Stderr: Job::getErrorOutput() will presumably never work, phpdbg sends (correctly) all output to stdout, even when in underlaying process prints to stderr.

Proposal A: skip getErrorOutput test for phpdbg.
Proposal B: remove getErrorOutput altogether from Job and return complete output in getOutput. Extensions of Job may implement another interface with getStandardOutput and getErrorOutput

Ansi codes in output could be fixed with this patch

src/Runner/ZendPhpDbgInterpreter.php:41
+ fwrite($pipes[0], "set colors 0\n");
+

Mikulas · 2015-12-08T22:04:46Z

Stderr: @JanTvrdik found it's actually fixed on php-7.0.1RC1 php/php-src@c1189ec

Ansi codes: alternative fix for noninteractive mode which is what Job invokes

src/Runner/ZendPhpDbgInterpreter.php:63
-       return $this->path . ' -qrr ' . $this->arguments;
+       return $this->path . ' -qrrb ' . $this->arguments;

Other than those bugs, are there any issues that would prevent getting this merged?

JanTvrdik · 2015-12-09T22:47:20Z

PING

milo · 2016-01-03T07:51:16Z

@JanTvrdik Would you squash last commit into the 1st and fix Job test in this way?

milo · 2016-01-06T12:55:18Z

Ping @JanTvrdik

dg · 2016-01-22T14:17:58Z

What about this? dg@e0a9875

Majkl578 · 2016-01-22T14:52:02Z

Just a note: you can be running PHPDBG SAPI while having XDebug enabled as well, but in that case XDebug is not what we want to use, ever, right?

milo · 2016-01-25T06:16:42Z

@dg Seems better.
@JanTvrdik Some attitude to this?

hrach · 2016-01-25T07:04:23Z

@milo why it seems better? It seems as a spagtheti code, badly testable, wired, ...

dg · 2016-01-25T07:27:13Z

It is only simplified, with fixed exception message. Still not sure about this #258 (comment)

JanTvrdik · 2016-01-25T11:27:33Z

yes, there should be check for PHP 7.0.0+; phpdbg in PHP 5.6 does not support code coverage
with @dg's refactoring it would be difficult to implement command line switch to manually force specific code coverage strategy, so it really depends on whether we see --coverage-strategy xdebug as useful or not

milo · 2016-01-25T12:49:42Z

I don't see the reason for choosing the coverage strategy manually for now and I would wait for real-world issue. In that case, David's simplification seems OK to me.

@JanTvrdik Would you add the version check and such simplifications?

JanTvrdik · 2016-01-26T18:48:38Z

@milo done

milo · 2016-01-27T06:15:01Z

src/Runner/CliTester.php

@@ -228,9 +230,6 @@ private function createRunner()
 	/** @return string */
 	private function prepareCodeCoverage()
 	{
-		if (!$this->interpreter->hasXdebug()) {
-			throw new \Exception("Code coverage functionality requires Xdebug extension (used {$this->interpreter->getCommandLine()})");
-		}


Imho, this should stay there with modified message. And as a workaround, ZendPhpDbgInterpreter::hasXDebug() should return TRUE.

That doesn't make any sense. PHPDBG may or may not have XDebug loaded, just like normal CLI/CGI SAPI.

The ZendPhpDbgInterpreter::hasXDebug() is used only for detection that Coverage is possible. I have a plan for refactoring.

@JanTvrdik Or maybe better ... && !$this->interpreter instanceof ZendPhpDbgInterpreter.

milo · 2016-01-27T06:15:39Z

@JanTvrdik One note in code. I'm sorry I didn't noticed earlier.

JanTvrdik · 2016-01-27T17:29:48Z

Everything related to hasXDebug should be refactored, but it is outside of the scope of this PR.

milo · 2016-01-27T17:32:26Z

Agree. ~~I thought only to return TRUE, nothing more.~~

Support for phpdbg with fast code coverage

milo · 2016-01-30T14:46:14Z

Thank you!

dg · 2016-02-05T00:29:10Z

src/Framework/Environment.php

@@ -62,7 +62,7 @@ public static function setupColors()
 	{
 		self::$useColors = getenv(self::COLORS) !== FALSE
 			? (bool) getenv(self::COLORS)
-			: (PHP_SAPI === 'cli' && ((function_exists('posix_isatty') && posix_isatty(STDOUT))
+			: ((PHP_SAPI === 'cli' || PHP_SAPI === 'phpdbg') && ((function_exists('posix_isatty') && posix_isatty(STDOUT))


Is PHP_SAPI === 'phpdbg' correct? PHP_SAPI is 'cli' when argument -qrrb -S cli is used.

@dg Handy when called phpdbg path/to/test.phpt manually.

fprochazka · 2016-02-07T12:56:09Z

Awesome work guys!

JanTvrdik force-pushed the phpdbg branch from 7c8c329 to 5a6f7d3 Compare October 2, 2015 09:48

milo added enhancement friendliness labels Oct 2, 2015

Majkl578 reviewed Oct 2, 2015
View reviewed changes

JanTvrdik force-pushed the phpdbg branch from 5a6f7d3 to b0a2c60 Compare October 15, 2015 21:47

dg reviewed Nov 9, 2015
View reviewed changes

JanTvrdik force-pushed the phpdbg branch 2 times, most recently from a25ae37 to 8e0f3b3 Compare December 9, 2015 11:32

JanTvrdik force-pushed the phpdbg branch from 5a4a13f to 6a538b9 Compare January 8, 2016 09:42

milo mentioned this pull request Jan 21, 2016

Unable to test multiple http codes within one TestCase #284

Closed

added support for phpdbg SAPI via ZendPhpDbgInterpreter

ee846c7

JanTvrdik force-pushed the phpdbg branch from 6a538b9 to ab20fd5 Compare January 25, 2016 18:29

milo reviewed Jan 27, 2016
View reviewed changes

JanTvrdik force-pushed the phpdbg branch 3 times, most recently from d045331 to 038cbed Compare January 30, 2016 14:00

CodeCoverage: added support for phpdbg strategy as alternative to Xdebug

9b62462

JanTvrdik force-pushed the phpdbg branch from 038cbed to 9b62462 Compare January 30, 2016 14:27

milo added a commit that referenced this pull request Jan 30, 2016

Merge pull request #258 from JanTvrdik/phpdbg

e227a3f

Support for phpdbg with fast code coverage

milo merged commit e227a3f into nette:master Jan 30, 2016

JanTvrdik deleted the phpdbg branch January 30, 2016 14:52

dg reviewed Feb 5, 2016
View reviewed changes

JanTvrdik mentioned this pull request May 27, 2016

Assert can find mismatch position when using patterns #314

Merged

Support for phpdbg with fast code coverage #258

Support for phpdbg with fast code coverage #258

Conversation

JanTvrdik commented Oct 2, 2015

Mikulas commented Oct 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Majkl578 commented Oct 2, 2015

milo commented Oct 3, 2015

JanTvrdik commented Oct 3, 2015

JanTvrdik commented Oct 3, 2015

milo commented Oct 3, 2015

Majkl578 commented Oct 16, 2015

hrach commented Nov 9, 2015

milo commented Nov 9, 2015

JanTvrdik commented Nov 9, 2015

hrach commented Nov 9, 2015

Majkl578 commented Nov 9, 2015

milo commented Nov 9, 2015

JanTvrdik commented Nov 9, 2015

milo commented Nov 9, 2015

hrach commented Nov 9, 2015

Majkl578 commented Nov 9, 2015

JanTvrdik commented Nov 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mikulas commented Dec 6, 2015

Mikulas commented Dec 8, 2015

JanTvrdik commented Dec 9, 2015

milo commented Jan 3, 2016

milo commented Jan 6, 2016

dg commented Jan 22, 2016

Majkl578 commented Jan 22, 2016

milo commented Jan 25, 2016

hrach commented Jan 25, 2016

dg commented Jan 25, 2016

JanTvrdik commented Jan 25, 2016

milo commented Jan 25, 2016

JanTvrdik commented Jan 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milo commented Jan 27, 2016

JanTvrdik commented Jan 27, 2016

milo commented Jan 27, 2016

milo commented Jan 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fprochazka commented Feb 7, 2016